[WIP] Add WidgetSet endpoint #968

lpichler · 2020-11-19T14:56:25Z

Fixes #953

This adds WidgetSet endpoint with CRUD.

LIST

GET /api/widget_sets
=>
{
    "name": "widget_sets",
    "count": 8,
    "subcount": 8,
    "pages": 1,
    "resources": [
        {
            "href": "http://localhost:3090/api/widget_sets/155"
        },
        {
            "href": "http://localhost:3090/api/widget_sets/161"
        },
        {
            "href": "http://localhost:3090/api/widget_sets/162"
        },
        {
            "href": "http://localhost:3090/api/widget_sets/168"
        },
        {
            "href": "http://localhost:3090/api/widget_sets/170"
        },
        {
            "href": "http://localhost:3090/api/widget_sets/152"
        },
        {
            "href": "http://localhost:3090/api/widget_sets/153"
        },
        {
            "href": "http://localhost:3090/api/widget_sets/154"
        }
    ],
    "actions": [
        {
            "name": "query",
            "method": "post",
            "href": "http://localhost:3090/api/widget_sets"
        },
        {
            "name": "create",
            "method": "post",
            "href": "http://localhost:3090/api/widget_sets"
        },
        {
            "name": "edit",
            "method": "post",
            "href": "http://localhost:3090/api/widget_sets"
        },
        {
            "name": "delete",
            "method": "post",
            "href": "http://localhost:3090/api/widget_sets"
        },
        {
            "name": "copy",
            "method": "post",
            "href": "http://localhost:3090/api/widget_sets"
        },
        {
            "name": "delete",
            "method": "delete",
            "href": "http://localhost:3090/api/widget_sets"
        }
    ],
    "links": {
        "self": "http://localhost:3090/api/widget_sets?offset=0",
        "first": "http://localhost:3090/api/widget_sets?offset=0",
        "last": "http://localhost:3090/api/widget_sets?offset=0"
    }
}

CREATE:

POST /api/widget_sets
{
    "name": "V",
    "description": "a",
    "set_data": { "col1": [7, 22, 20] },
    "group"   : {"id" : 1} # or href
}
=>
 {
    "results": [
        {
            "href": "http://localhost:3090/api/widget_sets/167",
            "id": "167",
            "name": "V",
            "description": "a",
            "set_type": "MiqWidgetSet",
            "created_on": "2020-12-09T15:29:35Z",
            "updated_on": "2020-12-09T15:29:35Z",
            "guid": "847f3d4f-9ccf-4049-9b60-d40a3d755bbd",
            "read_only": null,
            "set_data": {
                "col1": [
                    7,
                    22,
                    20
                ],
                "col2": [],
                "col3": [],
                "reset_upon_login": false,
                "locked": false
            },
            "mode": null,
            "owner_type": "MiqGroup",
            "owner_id": "1",
            "userid": null,
            "group_id": "1"
        }
    ]
}

EDIT:


POST /api/widget_sets/1
{
   "action" : "edit",
   "resource" : {
        "description": "testaa",
        "set_data": { "col1": [7, 22, 20] }
   } 
}

COPY

POST /api/widget_sets/167
{
   "action" : "copy",
   "resource" : {
        "name" : "ddd",
        "description": "testaa",
        "set_data": { "col1": [7, 22, 20] }
   } 
}
=>
{
    "success": true,
    "message": "Dashboard ddd successfully created.",
    "href": "http://localhost:3090/api/widget_sets/167"
}

DELETE

POST /api/widget_sets/167
{
   "action" : "delete"
}
=>

{
    "success": true,
    "message": "Dashboard Vd has been successfully deleted.",
    "href": "http://localhost:3090/api/widget_sets/169"
}

Links

Requires:

skateman · 2020-12-02T10:22:34Z

@lpichler we also need an endpoint for copying I think

https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/report_controller/dashboards.rb#L61

lpichler · 2020-12-02T10:44:40Z

@lpichler we also need an endpoint for copying I think

https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/controllers/report_controller/dashboards.rb#L61

It could be done by with create action but I can add it to simplify copy action.

skateman · 2020-12-02T12:49:30Z

@lpichler as there's a separate method on the model for copying, I'd rather expose it than simulate it with a create...

Fryguy · 2020-12-09T17:40:27Z

I'm not sure I like all of this in the API...feels like a lot of the setup should be in the model as class methods. @agrare @abellotti Thoughts? Is this a pattern we do elsewhere?

Fryguy · 2020-12-09T17:40:55Z

app/controllers/api/widget_sets_controller.rb

+    ALLOWED_FIELDS = REQUIRED_FIELDS_TO_COPY + %w[group description guid read_only set_data].freeze
+    SET_DATA_COLS = %i[col1 col2 col3].freeze
+
+    def init_set_data(set_data)


Anything that's not exposed to a route should be moved into a private method.

Fryguy · 2020-12-09T17:41:54Z

app/controllers/api/widget_sets_controller.rb

+
+    def validate_description(group_id, description)
+      widget_set_exists_in_group = MiqWidgetSet.exists?(:owner_id => group_id, :description => description)
+      raise ArgumentError, "Description(Tab Title) must be unique for this group" if widget_set_exists_in_group


Like this stuff should be model validations...not in the API.

Fryguy · 2020-12-09T17:42:19Z

app/controllers/api/widget_sets_controller.rb

+    def create_resource(type, id = nil, data = nil)
+      raise_if_unsupported_fields_passed(data)
+
+      raise ArgumentError, "Name cannot contain \"|\"" if data['name'].index('|')


Also should be a model validation.

Agreed, API does not raise any ArgumentErrors today, those come from the model.

lpichler · 2020-12-09T17:54:48Z

I'm not sure I like all of this in the API...feels like a lot of the setup should be in the model as class methods. @agrare @abellotti Thoughts? Is this a pattern we do elsewhere?

Yes, this is exactly what I wanted to discuss - I am not against, I wanted to see opinions from others.
This is common that there is lot of validations of various kinds in controllers and when we are creating API endpoints we have to decide if validations should go to in API or to model level(and we can rely that data in db are already valid ).

cc @kbrock

abellotti · 2020-12-09T20:08:26Z

app/controllers/api/widget_sets_controller.rb

+      new_set_data[:reset_upon_login] = !!set_data['reset_upon_login']
+      new_set_data[:locked] = !!set_data['locked']
+      new_set_data
+    end


I'm confused with this, is the API defining a structure beyond the basic MiqWidgetSet ? It almost feels we need a new subclass of MiqWidgetSet in the model for what we're building/managing here and move most of this logic in there.

From Pull Request: ManageIQ/manageiq-api#968

lpichler · 2020-12-16T13:34:36Z

@miq-bot cross-repo-tests ManageIQ/manageiq#20890

From Pull Request: ManageIQ/manageiq-api#968

miq-bot · 2020-12-16T15:32:16Z

Checked commit lpichler@4dea6a4 with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
2 files checked, 4 offenses detected

app/controllers/api/widget_sets_controller.rb

❗ - Line 14, Col 29 - Naming/MethodParameterName - Method parameter must be at least 3 characters long.
❗ - Line 20, Col 31 - Naming/MethodParameterName - Method parameter must be at least 3 characters long.
❗ - Line 32, Col 29 - Naming/MethodParameterName - Method parameter must be at least 3 characters long.
❗ - Line 6, Col 31 - Naming/MethodParameterName - Method parameter must be at least 3 characters long.

abellotti · 2021-01-20T16:33:30Z

app/controllers/api/widget_sets_controller.rb

+module Api
+  class WidgetSetsController < BaseController
+    REQUIRED_FIELDS_TO_COPY = %w[name].freeze
+    ALLOWED_FIELDS = REQUIRED_FIELDS_TO_COPY + %w[group description guid read_only set_data].freeze


with 'read_only' here, does this mean the API can create read_only widget sets and cannot delete them ? I usually think of read_only as set to true for some system seeded resources.

abellotti · 2021-01-20T16:39:34Z

needs a bulk query test. can be added in spec/requests/collections_spec.rb

miq-bot · 2021-02-16T17:10:49Z

This pull request is not mergeable. Please rebase and repush.

From Pull Request: ManageIQ/manageiq-api#968

kavyanekkalapu · 2021-11-17T20:14:18Z

@jrafanie This is needed for Overview/Reports/Dashboards -> Copy Selected Dashboard Missing endpoint for manipulating with dashboards form. Right now we are using miqajaxmethod as we don't have api.

lpichler added the enhancement label Nov 19, 2020

lpichler requested review from abellotti and bdunne as code owners November 19, 2020 14:56

lpichler force-pushed the add_widget_set_endpoint branch 2 times, most recently from c89a6cc to 8dcddfa Compare November 19, 2020 15:38

agrare requested review from agrare and gtanzillo November 19, 2020 18:49

agrare assigned gtanzillo Nov 19, 2020

lpichler changed the title ~~Add WidgetSet endpoint~~ [WIP] Add WidgetSet endpoint Dec 2, 2020

miq-bot added the wip label Dec 2, 2020

lpichler force-pushed the add_widget_set_endpoint branch 3 times, most recently from 0103ab1 to adea740 Compare December 9, 2020 15:55

lpichler changed the title ~~[WIP] Add WidgetSet endpoint~~ Add WidgetSet endpoint Dec 9, 2020

lpichler removed the wip label Dec 9, 2020

lpichler mentioned this pull request Dec 9, 2020

[Lasker] API additions #977

Closed

10 tasks

Fryguy reviewed Dec 9, 2020

View reviewed changes

abellotti reviewed Dec 9, 2020

View reviewed changes

lpichler changed the title ~~Add WidgetSet endpoint~~ [WIP] Add WidgetSet endpoint Dec 10, 2020

miq-bot added the wip label Dec 10, 2020

lpichler force-pushed the add_widget_set_endpoint branch from adea740 to 39423a2 Compare December 11, 2020 14:49

lpichler mentioned this pull request Dec 11, 2020

Add validations for MiqWidgetSet ManageIQ/manageiq#20890

Merged

lpichler force-pushed the add_widget_set_endpoint branch from 39423a2 to 1195455 Compare December 11, 2020 15:03

lpichler force-pushed the add_widget_set_endpoint branch from 1195455 to f1b0339 Compare December 16, 2020 11:26

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Dec 16, 2020

Running tests for lpichler

793ccca

From Pull Request: ManageIQ/manageiq-api#968

miq-bot mentioned this pull request Dec 16, 2020

[BOT] Cross repo test for ManageIQ/manageiq-api#968 ManageIQ/manageiq-cross_repo-tests#249

Closed

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Dec 16, 2020

Running tests for lpichler

fd5a5f0

From Pull Request: ManageIQ/manageiq-api#968

miq-bot mentioned this pull request Dec 16, 2020

[BOT] Cross repo test for ManageIQ/manageiq-api#968 ManageIQ/manageiq-cross_repo-tests#250

Closed

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Dec 16, 2020

Running tests for lpichler

0ee7897

From Pull Request: ManageIQ/manageiq-api#968

miq-bot mentioned this pull request Dec 16, 2020

[BOT] Cross repo test for ManageIQ/manageiq-api#968 ManageIQ/manageiq-cross_repo-tests#251

Closed

Add widget set endpoint

4dea6a4

lpichler force-pushed the add_widget_set_endpoint branch from f1b0339 to 4dea6a4 Compare December 16, 2020 15:16

gtanzillo assigned Fryguy, gtanzillo and abellotti and unassigned gtanzillo Jan 20, 2021

abellotti reviewed Jan 20, 2021

View reviewed changes

miq-bot added the unmergeable label Feb 16, 2021

chessbyte mentioned this pull request Feb 23, 2021

[Najdorf] API Interface Changes #1016

Closed

7 tasks

lpichler closed this Jul 13, 2021

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Jul 13, 2021

Running tests for lpichler

f522b73

From Pull Request: ManageIQ/manageiq-api#968

miq-bot mentioned this pull request Jul 13, 2021

[BOT] Cross repo test for ManageIQ/manageiq-api#968 ManageIQ/manageiq-cross_repo-tests#442

Closed

miq-bot pushed a commit to ManageIQ/manageiq-cross_repo-tests that referenced this pull request Jul 13, 2021

Running tests for lpichler

ada8298

From Pull Request: ManageIQ/manageiq-api#968

miq-bot mentioned this pull request Jul 13, 2021

[BOT] Cross repo test for ManageIQ/manageiq-api#968 ManageIQ/manageiq-cross_repo-tests#443

Closed

kbrock assigned kbrock and unassigned gtanzillo and abellotti Dec 3, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add WidgetSet endpoint #968

[WIP] Add WidgetSet endpoint #968

lpichler commented Nov 19, 2020 •

edited

Loading

skateman commented Dec 2, 2020 •

edited

Loading

lpichler commented Dec 2, 2020

skateman commented Dec 2, 2020

Fryguy commented Dec 9, 2020

Fryguy Dec 9, 2020

Fryguy Dec 9, 2020

Fryguy Dec 9, 2020

abellotti Dec 9, 2020

lpichler commented Dec 9, 2020

abellotti Dec 9, 2020

lpichler commented Dec 16, 2020

miq-bot commented Dec 16, 2020

abellotti Jan 20, 2021

abellotti commented Jan 20, 2021

miq-bot commented Feb 16, 2021

kavyanekkalapu commented Nov 17, 2021

[WIP] Add WidgetSet endpoint #968

[WIP] Add WidgetSet endpoint #968

Conversation

lpichler commented Nov 19, 2020 • edited Loading

LIST

CREATE:

EDIT:

COPY

DELETE

Links

skateman commented Dec 2, 2020 • edited Loading

lpichler commented Dec 2, 2020

skateman commented Dec 2, 2020

Fryguy commented Dec 9, 2020

Fryguy Dec 9, 2020

Choose a reason for hiding this comment

Fryguy Dec 9, 2020

Choose a reason for hiding this comment

Fryguy Dec 9, 2020

Choose a reason for hiding this comment

abellotti Dec 9, 2020

Choose a reason for hiding this comment

lpichler commented Dec 9, 2020

abellotti Dec 9, 2020

Choose a reason for hiding this comment

lpichler commented Dec 16, 2020

miq-bot commented Dec 16, 2020

abellotti Jan 20, 2021

Choose a reason for hiding this comment

abellotti commented Jan 20, 2021

miq-bot commented Feb 16, 2021

kavyanekkalapu commented Nov 17, 2021

lpichler commented Nov 19, 2020 •

edited

Loading

skateman commented Dec 2, 2020 •

edited

Loading